Skip to content

CLN/PERF: remove unnecessary ensure_platform_int #44563

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

jorisvandenbossche
Copy link
Member

This line was added in #43977, while the docstring and type information (as added in #43977) indicates this is already the case.

@jbrockmendel
Copy link
Member

we have a lot of should-be-unnecessary ensure_platform_int calls. these should be extremely cheap, so haven't been bothering to remove.

@jorisvandenbossche
Copy link
Member Author

so haven't been bothering to remove.

And in the past I bothered making a fast-path take_1d which explicitly removed it. It did show up in reindex benchmarks IIRC (#40246, #39692)

@jorisvandenbossche
Copy link
Member Author

Digging up the benchmarks. Doing one specific for take_1d:

from pandas.core.array_algos.take import take_1d
arr = np.arange(10000)
idx = np.arange(100, dtype=np.intp)

In [5]: %timeit take_1d(arr, idx, allow_fill=False)
498 ns ± 0.77 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  # <-- master
465 ns ± 0.962 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)  # <-- PR

This is of course about nanoseconds, so in absolute terms only a tiny difference. But because this function is used repeatedly in things like unstack and reindex, it does make a noticeable difference. Eg on the unstack case as used in #40246, it gives ~ 3-5% difference (which is still small, but IMO worth it since this is actually removing a single line, and the constraint is explicitly documented / in the type information)

@jbrockmendel
Copy link
Member

sure no objection

@jreback jreback added 32bit 32-bit systems Performance Memory or execution speed performance labels Nov 21, 2021
@jreback jreback added this to the 1.4 milestone Nov 21, 2021
@jreback
Copy link
Contributor

jreback commented Nov 21, 2021

no objection, can you rebase

@jorisvandenbossche jorisvandenbossche merged commit 3a25cb9 into pandas-dev:master Nov 22, 2021
@jorisvandenbossche jorisvandenbossche deleted the am-take1d-novalidateindices branch November 22, 2021 08:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
32bit 32-bit systems Performance Memory or execution speed performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants